Support voltage formulas#7
Merged
Merged
Conversation
shsms
added
the
cmd:skip-release-notes
Collaborator
Author
|
Draft because it depends on unmerged code from the component graph repo. |
Collaborator
Author
|
And based on #6 |
Collaborator
Author
|
Ready for review now. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for voltage formulas by implementing a major internal refactoring of the metric representation and component graph access patterns. The external interface maintains compatibility with only minor changes, while internally introducing a trait-based approach that differentiates between aggregation and coalesce formulas based on metric types.
- Refactored metrics from enum to trait-based system with
AcMetrictrait and individual metric structs - Introduced separate
AggregationFormulaandCoalesceFormulatypes with voltage metrics using coalesce behavior - Updated logical meter actor to track formulas and resamplers by both formula string and metric type
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/logical_meter/metric/metric_trait.rs | Defines new AcMetric trait for type-safe metric handling |
| src/logical_meter/metric.rs | Converts enum-based metrics to trait-based system using macro generation |
| src/logical_meter/logical_meter_handle.rs | Updates public API to use generic methods with trait bounds |
| src/logical_meter/logical_meter_actor.rs | Modifies internal tracking to use composite keys of (formula, metric) |
| src/logical_meter/formula/graph_formula_provider.rs | Implements trait for generating formulas from component graph |
| src/logical_meter/formula/coalesce_formula.rs | New formula type for voltage metrics using coalesce operations |
| src/logical_meter/formula/aggregation_formula.rs | Extracted aggregation formula with arithmetic operations |
| src/logical_meter/formula.rs | Refactored to trait-based formula system |
| src/logical_meter.rs | Updated module exports |
| src/lib.rs | Updated public API exports |
| examples/logical_meter.rs | Updated example to use new metric syntax |
| Cargo.toml | Updated component graph dependency version |
| use crate::{Error, Metric, Sample}; | ||
| use crate::{Error, Sample}; | ||
| use tokio::sync::broadcast; | ||
|
|
There was a problem hiding this comment.
The trait method uses impl Future return type which requires importing std::future::Future. This import is missing and will cause compilation errors.
Suggested change
| use std::future::Future; |
| macro_rules! graph_formula_provider { | ||
| ($(($fnname:ident $(, $idsparam:ident)?)),+ $(,)?) => {$( | ||
|
|
||
| fn $fnname<M: crate::metric::metric_trait::AcMetric>( |
There was a problem hiding this comment.
The macro uses an absolute path crate::metric::metric_trait::AcMetric but the relative import path should be used consistently with other parts of the file. Consider using crate::logical_meter::metric::metric_trait::AcMetric or a relative import.
Suggested change
| fn $fnname<M: crate::metric::metric_trait::AcMetric>( | |
| fn $fnname<M: super::metric::metric_trait::AcMetric>( |
| macro_rules! impl_graph_formula_provider { | ||
| ($(($fnname:ident, $graphfnname:ident$(, $idsparam:ident)?)),+ $(,)?) => {$( | ||
|
|
||
| fn $fnname<M: crate::metric::metric_trait::AcMetric>( |
There was a problem hiding this comment.
Same issue as above - the macro uses an inconsistent import path crate::metric::metric_trait::AcMetric instead of the correct module path.
Suggested change
| fn $fnname<M: crate::metric::metric_trait::AcMetric>( | |
| fn $fnname<M: AcMetric>( |
niklas-timpe
previously approved these changes
Jul 18, 2025
| [dependencies] | ||
| chrono = "0.4" | ||
| frequenz-microgrid-component-graph = { git = "https://github.com/frequenz-floss/frequenz-microgrid-component-graph-rs.git", rev = "ab3b998" } | ||
| # frequenz-microgrid-component-graph = { git = "https://github.com/shsms/frequenz-microgrid-component-graph-rs.git", branch = "coalesce-formulas" } |
Collaborator
There was a problem hiding this comment.
should that stay here?
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Because there will be another type of formula that doesn't support aggregation. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
This will be implemented by the upcoming `CoalesceFormula` as well. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
This also enables us to delegate the component graph formula creation to the logical meter formulas, through the `GraphFormulaProvider` trait. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
And introduce support for voltage and frequency metrics using `CoalesceFormula`s. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
This is a bugfix, because earlier there was only one resampler for each component and data from all metrics were coming from that one resampler, which had the data only for the first metric that was subscribed to. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
The channel used to communicate the status of a component data stream was called `stream_stopped_tx/rx`, which is no longer accurate. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Earlier we were picking a random metric and sending its data to all formulas, which was incorrect. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is also a major internal revamp of how metrics are represented and how the component graph is accessed, but the external interface only has minor changes.